fix(iOS): onNativeDismissCancelled called too early during modal dismissal #2129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @zetavg, thanks for preparing these changes 🎉. I took a glance & these changes look good, however I'll need to find time to test this! Will get back you once I do. |
|
@kkafar Giving myself a notification to take care of this PR somewhere next week. |
kkafar
approved these changes
Aug 6, 2024
There was a problem hiding this comment.
I finally got to test the changes, sorry for such delay. I've merged main & merged this implementation with recent changes from #2184
Note
We should deprecate emitting gestureCancel in presentationControllerDidAttemptToDismiss in favour of emitting only dismissCancelled to align fully with react-navigation v7 API. Created ticket for this.
Thank you for your contribution @zetavg!
|
Will merge once the CI passes. |
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…issal (software-mansion#2129) ## Description Currently, on iOS, `onNativeDismissCancelled` is being called while the user begins to drag down a modal (which calls the callback function of the `usePreventRemove` hook, if `preventRemove` is `true`). This is not a common behavior of iOS apps and can sometimes be annoying: the callback will be triggered while the user is scrolling around a `ScrollView` in the modal and has hit the top, which is interrupting. | Ideal | Before | After | |-------|--------|-------| | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/167828c8-eb3a-4b4b-99a7-b9eb7ec55f06"><img alt="Ideal" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/fca2e909-a523-422a-80a5-3ac301539486" /></a> | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/6c166757-8c73-4e90-9236-9faf84514a14"><img alt="Before" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/812426eb-5e5d-4da5-855b-8ff6d1af14ef" /></a> | <a href="https://github.com/software-mansion/react-native-screens/assets/3784687/7993f36c-3455-4b23-8a28-f46956655f8a"><img alt="After" width="240" src="https://github.com/software-mansion/react-native-screens/assets/3784687/62ab528a-4675-44fe-a5ae-682c2d155817" /></a> | | The event fires when the user **releses** the drag. If the user did not intend to dismiss the modal (the user did not drag it down far enough or dragged it back to its original position before release), the event will not fire. | The event fires as soon as the modal is dragged down, regardless of whether the user intends to dismiss the modal or not. (In this case, the user just wants to scroll to the top.) | Same as "Ideal". | This PR adjusts this to the more ideal behavior. ## Changes Move the call of `notifyDismissCancelled` from [`presentationControllerShouldDismiss`](https://developer.apple.com/documentation/uikit/uiadaptivepresentationcontrollerdelegate/3229890-presentationcontrollershoulddism) to [`presentationControllerDidAttemptToDismiss`](https://developer.apple.com/documentation/uikit/uiadaptivepresentationcontrollerdelegate/3229888-presentationcontrollerdidattempt) - which "Notifies the delegate that a user-initiated attempt to dismiss a view was prevented", and has the same platform avalibality as `presentationControllerShouldDismiss`. ## Test code and steps to reproduce Use the new "Prevent Remove" example: <img width="300" src="https://github.com/software-mansion/react-native-screens/assets/3784687/622673dc-d520-4828-a2c3-83de7f2e3bef" /> ### Full test recordings **Modal** https://github.com/software-mansion/react-native-screens/assets/3784687/16777817-3949-413c-83ad-f0f7136af158 **Normal Screen** (this should not be affected since I believe that preventing going back from a normal screen is not handled here) https://github.com/software-mansion/react-native-screens/assets/3784687/41f25247-eb54-4ce8-9348-690d0a925e8f ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, on iOS,
onNativeDismissCancelledis being called while the user begins to drag down a modal (which calls the callback function of theusePreventRemovehook, ifpreventRemoveistrue).This is not a common behavior of iOS apps and can sometimes be annoying: the callback will be triggered while the user is scrolling around a
ScrollViewin the modal and has hit the top, which is interrupting.This PR adjusts this to the more ideal behavior.
Changes
Move the call of
notifyDismissCancelledfrompresentationControllerShouldDismisstopresentationControllerDidAttemptToDismiss- which "Notifies the delegate that a user-initiated attempt to dismiss a view was prevented", and has the same platform avalibality aspresentationControllerShouldDismiss.Test code and steps to reproduce
Use the new "Prevent Remove" example:
Full test recordings
Modal
Full.Test.-.Modal.-.05.mp4
Normal Screen (this should not be affected since I believe that preventing going back from a normal screen is not handled here)
Full.Test.-.Screen.-.05.mp4
Checklist